Closed
Bug 713577
Opened 13 years ago
Closed 13 years ago
Clang Static Analysis: Operand in multiplication is garbage value in content/svg/content/src/nsSVGFilters.cpp
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
INVALID
People
(Reporter: decoder, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
1.54 KB,
patch
|
Details | Diff | Splinter Review |
The following (almost identical) reports have been generated by static analysis using Clang:
http://users.own-hero.net/~decoder/mozilla/report/2011-12-21-1/report-Nd3e7L.html#EndPath
http://users.own-hero.net/~decoder/mozilla/report/2011-12-21-1/report-yCjuGu.html#EndPath
http://users.own-hero.net/~decoder/mozilla/report/2011-12-21-1/report-R2SqdD.html#EndPath
It would be good if someone familiar with the particular code could check if
- this is really a bug or a false positive
- and/or if it makes sense to adjust the code (even if there is not a real bug present, e.g. by adding a missing initialization).
In these particular reports, the problems seems to be that depending on the value of | values.Length(); |, one of the multiplications later may be using uninitialized/garbage values. The length assumed by the analysis can be seen in line 1203, depending on how many times the loop is executed.
![]() |
||
Comment 1•13 years ago
|
||
The code ensures that values.Length() == 20 and that all 20 entries of colorMatrix are initialized. That's the loop on lines 1203 and 1204.
The loop condition on line 1276 ensures the following invariants:
1) 0 <= i < 4
2) row = 5*i.
So the maximal value of |row| is 15. Hence row+3 and row+4 are at most 18 and 19, which are inside the initialized colorMatrix array (the last two entries of it, in fact).
So this looks like a false-positive to me at first glance.
I really don't know how to make clang not flag this offhand short of changing the line 1276 loop condition to compare row to 20 instead of i to 4, maybe?
It may be more worthwhile to see whether the clang bug can be fixed.
Reporter | ||
Comment 2•13 years ago
|
||
First of all, thanks for looking into this :)
(In reply to Boris Zbarsky (:bz) from comment #1)
> The code ensures that values.Length() == 20
Where is that ensured? It's probably just that what's being missed by the analysis, because it assumes | values.Length() | to me smaller (and in that case, the result would be correct).
Comment 3•13 years ago
|
||
The value of values.Length() is checked somewhat higher up in a switch statement. If type == SVG_FECOLORMATRIX_TYPE_MATRIX then we have this:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGFilters.cpp#1200
All 20 values in the colorMatrix are always initialised. And the index never exceeds 19
i < 4 therefore row < 15 and so row + 4 < 19
It looks like the problem is that it is not figuring out that the two calls to Length return the same value. The attached patch hides the warning.
I will try figure out if this can be improved on the clang side.
I reported llvm.org/pr11658.
Comment 6•13 years ago
|
||
Doesn't seem valid as an SVG bug.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•